Repository navigation
fix(kubernetes): preserve sandbox ownership when seeding workspace - #3208
loveRhythm1990 wants to merge 1 commit into
Conversation
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This localized Kubernetes workspace-seeding fix is project-valid and the user-facing documentation is updated, but one storage-backend regression must be addressed before CI handoff.
Action required: @loveRhythm1990, make ownership restoration fall back safely when the destination filesystem rejects chown, and add a constrained behavioral regression test.
Blocking findings:
GATOR-f5508202-01: ownership restoration can prevent sandbox startup on writable filesystems that reject ownership changes.
Carried findings:
- None
Gator metadata
- Validation: Localized bug fix linked to #2761 with a concrete production path, reproduction, and no duplicate candidate.
- Docs: Fern compute-driver reference updated; navigation already covers the existing page.
- Checks: Required current-head workflows have not started; copy-pr validation is still pending.
- E2E:
test:e2eand Kubernetes-specific coverage are required after review feedback is resolved; not dispatched yet. - Head SHA:
f5508202e5de30185d8e79154dec3712947f9b87 - Base SHA:
320d4ef79dd572c642133f175f12bafc20d89fd9 - Merge base SHA:
320d4ef79dd572c642133f175f12bafc20d89fd9 - Patch ID:
f1044b20935f30a89efda6aa8eec7cba011ea349 - Gator payload:
8 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
The workspace-init container seeds a fresh PVC as root and extracted with --no-same-owner, so every seeded path landed owned by uid 0. A mode-0700 home directory shipped by the image (~/.config, ~/.cache) was then unreachable for the workload. fsGroup does not compensate: kubelet applies it when the volume is mounted, which is before the init container writes anything. Under the sidecar topology the supervisor's privileged workspace reconciliation does not run, so the seeded ownership is load-bearing. Rewrite ownership to the resolved sandbox identity while building the transfer archive instead. Seeding stays root, so images that ship root-owned private content still seed, and no recursive chown runs over the workspace (the pattern that broke on read-only submounts in NVIDIA#2294). Modes and timestamps are still not restored. Two environments cannot restore ownership, and neither may turn a permissions problem into a pod that will not start. --owner/--group are GNU extensions and this init container runs the sandbox image itself, so the script probes tar once and drops to the previous flags on a minimal base such as Alpine, which provides BusyBox tar. A writable backend can also accept the write but reject chown, root-squashed NFS being the usual case; extraction retries without ownership, and the sentinel is recorded when either attempt succeeds so a workspace that never seeded is never marked initialized. The script builder now takes its source and destination paths as arguments so the retry and sentinel logic can be driven directly in tests with a stub tar. A real tar cannot cover it: ownership is only restored under uid 0, so an unprivileged run skips the chown and always succeeds. Signed-off-by: loveRhythm1990 <qiuweimin@126.com>
f550820 to
3ed73e4
Compare
|
Label |
|
/ok to test 3ed73e4 |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @loveRhythm1990. I checked the denied-chown retry you described, the sentinel control flow, and the stub-tar cases on the latest head. The earlier storage-backend blocker is resolved: fallback extraction now succeeds without ownership restoration, while total extraction failure leaves the sentinel absent. No blocking findings remain.
Blocking findings:
- No blocking findings remain
Carried findings:
GATOR-f5508202-01: resolved by the current-head fallback and behavioral coverage
Gator metadata
- Validation: Localized Kubernetes workspace-seeding bug fix linked to #2761 with a concrete production path and reproduction.
- Docs: Fern compute-driver reference and architecture documentation are updated; existing navigation already covers the page.
- Checks: Current-head Branch Checks and E2E workflows are dispatched; Helm Lint has run and required gates are still settling.
- E2E:
test:e2eapplied;/ok to testcreated the current-head mirror and Branch E2E Checks is queued. - Head SHA:
3ed73e43bb96592b47de9d916ed785a2a4d8d993 - Base SHA:
02b664bb0d978ac0baec9aaa0bf06a2a4f67e83d - Merge base SHA:
02b664bb0d978ac0baec9aaa0bf06a2a4f67e83d - Patch ID:
e6a4699d3f8ef29c30977250eca60d3eda74be5c - Gator payload:
8 - Review mode:
follow_up - Previous reviewed SHA:
f5508202e5de30185d8e79154dec3712947f9b87 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
|
I'm closing this as internal discussions determined we should be able to test new architecture and revisit this if this is still an issue |
Monitoring CompleteMonitoring is complete because this PR has been closed without merge following the maintainer's decision to test the new architecture first and revisit this change if the issue remains. Final status: The latest Gator review found no remaining blocking findings on the current head. CI monitoring had been dispatched, but the PR closed before a merge decision. I removed the active Gator metadata
|
Summary
Workspace PVC seeding wrote every path as root, so a mode-
0700home directoryshipped by the sandbox image (
~/.config,~/.cache) was unreachable for theworkload under the sidecar topology. This rewrites ownership to the resolved
sandbox identity while building the transfer archive, so seeded content is
usable on first boot.
Related Issue
Fixes #2761.
The issue is still
state:triage-needed, so this is submitted as an obviouslocalized bug fix rather than accepted work: it changes one init-container
command string in the Kubernetes driver, adds no configuration surface, and
introduces no new behavior beyond correcting the seeded ownership. The reported
behavior was independently reproduced on the issue by @jiridanek. Happy to hold
this until the issue is triaged if maintainers prefer that order.
Changes
apply_workspace_persistencetakes the resolvedsandbox_uidalongside theexisting
sandbox_gid, and the init script builds the transfer archive with--owner/--group/--numeric-ownerso extraction restores the sandboxidentity instead of root.
Rewriting at archive time — rather than seeding as the sandbox user, or
chowning the tree afterwards — keeps root's ability to read every source path,
so images that ship root-owned private content still seed. It also avoids a
recursive chown over the workspace, the pattern that broke on read-only
submounts in bug: Kubernetes sandbox crashes EROFS — recursive
chown /sandboxfails on read-only submounts (0.0.82) #2294. Modes and timestamps are still not restored, so a nestedread-only mount under the workspace is never chmod'ed during seeding.
--owner/--groupare GNU extensions, and this init container runs thesandbox image itself. A minimal base such as Alpine seeds with BusyBox
tar,which rejects them; without a guard the init container would exit non-zero and
the pod would never start. The script probes
taronce and falls back to theprevious flags, so those images keep the behavior they have today.
architecture/compute-runtimes.md: documents the seeding ownership contractand the fallback.
docs/reference/sandbox-compute-drivers.mdx: corrects the resolved-identitylist. It claimed the resolved UID/GID appear in the "PVC init container
securityContext.runAsUser/runAsGroup/fsGroup", but that container isrunAsUser: 0with norunAsGroup, andfsGroupis a pod-level field. Theentry now describes where the identity actually lands, including the BusyBox
caveat.
Testing
mise run pre-commitpassesUnit tests (
cargo test -p openshell-driver-kubernetes, 230 passed):workspace_init_seeds_content_owned_by_the_sandbox_identity— asserts theidentity/permission contract: ownership is rewritten, numerically, and no
chownwalks the tree.workspace_init_ownership_tracks_resolved_identity— drives the fullsandbox_template_to_k8spath with an OpenShift-style UID (1000660000) sothe seeded identity is not pinned to
1000.workspace_init_falls_back_when_tar_lacks_ownership_extensions— asserts theprobe and the fallback flags.
workspace_init_command_checks_sentinel— unchanged idempotency andno-metadata-restore coverage.
Behavioral verification: I extracted the script this code actually generates and
ran it as root in two containers, against a PVC root prepared the way kubelet
leaves it (
root:1000, mode2770, setgid).GNU tar image (Ubuntu 24.04 with a LibreOffice-created mode-
0700~/.config) — before and after:Re-running the init script with the sentinel present exits 0 and leaves a file
written into
.configafter seeding untouched.BusyBox
tar(busybox:latest, v1.38.0), the regression this guards against:E2E gap. There is no existing e2e coverage of workspace PVC seeding to
extend —
e2e/rust/tests/workspace_*.rscover the workspace/namespace API, andthe only PVC reference in
e2e/with-kube-gateway.shis cleanup. A first-bootcase as described in the issue's acceptance criteria needs a purpose-built
sandbox image shipping a mode-
0700workspace directory, published where thekind-based lane can pull it. I did not want to bundle that into this fix without
guidance, especially as
test:e2e-kubernetesis currently an optional gate.Happy to add it here or in a follow-up — maintainer's call.
Checklist